✅ Deploy Preview for scrc-coding-guidelines ready!

Name	Link
🔨 Latest commit	84c32f4
🔍 Latest deploy log	https://app.netlify.com/projects/scrc-coding-guidelines/deploys/697fcdbeae7e540008ba1a4a
😎 Deploy Preview	https://deploy-preview-368--scrc-coding-guidelines.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

I opened a Zulip thread to ask about this: How should one combine unsafe and declarative macros?.

I wanted to know how useful this pattern (of having an unsafe block inside of a macro's code) is.

And it seems like it is quite useful, and necessary in some cases:

A very simple example by scottmcm:

let slice = $crate::MyUnsafeTrait::get_me_the_thing($e);
// SAFETY: `MyUnsafeTrait` promises that `get_me_the_thing` returns a non-empty slice
unsafe { slice.get_unchecked(0) }

Another example, this time from std (explained by y21):

Are there things that this unlocks, that are otherwise impossible or unreasonable to achieve with e.g. invoking the macro inside of an unsafe block instead?

I think the pin! macro is also a good example. It calls the unsafe function Pin::new_unchecked in an unsafe block, and it can do so soundly because it takes ownership of the value and only gives back a Pin<&mut _>, ensuring that it stays pinned forever. This only works because it's a macro, and there's no reason to require an unsafe block at the use site because the safety contract of Pin::new_unchecked is upheld by the macro itself; there's nothing that a user needs to uphold when invoking this macro. So an unsafe block inside of the macro is perfectly valid there

@felix91gr I think we can both seen the value or need for this rule?

maybe we can make some adjustments around valid use cases.

Something like:

You can have an unsafe macro with an arbitrary identifier like the pin! macro if the macro provided the macro upholds the safety contract and there is no way to misuse the macro to introduce undefined behavior.

If you have an unsafe macro that exposes unsafe behavior you must prefix the macro name with the string "unsafe_"

Unsafe macros without a prefix are not allowed.

Or you could break this up into a strict advisory rule and a less strict required rule.

It's not great when the language / library violates your rule.

MISRA does this with rules like don't use restrict which is used throughout the C library, but it's mostly because they don't know what they are talking about.

You can have an unsafe macro with an arbitrary identifier like the pin! macro if the macro provided the macro upholds the safety contract and there is no way to misuse the macro to introduce undefined behavior.

I agree. This seems very reasonable: we would be classifying those, so-to-say, as "safe macros which internally have unsafe code".

In Rust, for safe APIs that contain unsafe code within, the contract they must uphold is that there must be no way of calling them from safe code and introduce Undefined Behavior.

So, this idea would be following the same kind of standard, only for macros instead of functions.

The burden of proof is higher (one must prove that any use of the macro cannot introduce UB), but if done properly (like we believe to be the case for the pin! macro), it seems like a reasonable and scalable approach.

It would also only require local reasoning about safety which is good. It would be much like how properly encapsulating unsafe code within safe APIs allows for local reasoning about them, and thus, scalability of it.

If you have an unsafe macro that exposes unsafe behavior you must prefix the macro name with the string "unsafe_"

Continuing with the theme above, this seems very reasonable as well. It would be the macro equivalent of an unsafe function: "if you call this macro (/ fn), you have to make sure the x, y and z invariants are preserved.", as well as "you may only call this macro (/ fn) from within an unsafe block".

Which are themselves the rules for unsafe functions (hence why the (/ fn))

Unsafe macros without a prefix are not allowed.

That's another parallel to unsafe functions: "if a macro (/fn) requires to manually preserve invariants (to avoid causing UB from the code it expands), then it must be labeled as such". In the case of functions, there's the unsafe keyword to label them as such. For macros, we don't have that, but for now a prefix in the name (+ linting) should be enough.

It's not great when the language / library violates your rule.

MISRA does this with rules like don't use restrict which is used throughout the C library, but it's mostly because they don't know what they are talking about.

🤣 it makes me smile to see you dunk on MISRA, dunno why

In their defense: does gcc have full and sound restrict support? I know LLVM only supports a small subset of it. And back when it had broader support for it, it was incredibly broken.

Given that history (and how hard a time LLVM at least has had trying to fully implement it), I would not be surprised if a broad ban on restrict were raised in the past.

👋 Hey @mdabir1203! You've been assigned to review this coding guideline PR.

Your Role as Reviewer

As outlined in our contribution guide, please:

1. Begin your review within 14 days
2. Provide constructive feedback on the guideline content, examples, and formatting
3. Iterate with @rcseacord - they may update the PR based on your feedback
4. When the guideline is ready, approve and add to the merge queue

Review Checklist

• Guideline title is clear and follows conventions
• Amplification section expands on the title appropriately
• Rationale explains the "why" effectively
• Non-compliant example(s) clearly show the problem
• Compliant example(s) clearly show the solution
• Code examples compile (check the CI results)
• FLS paragraph ID is correct

Bot Commands

If you need to pass this review:

• @guidelines-bot /pass [reason] - Pass just this PR to the next reviewer
• @guidelines-bot /away YYYY-MM-DD [reason] - Step away from the queue until a date
• @guidelines-bot /release [@username] [reason] - Release assignment (yours or someone else's with triage+ permission)

To assign someone else:

• @guidelines-bot /r? @username - Assign a specific reviewer
• @guidelines-bot /r? producers - Request the next reviewer from the queue

Other commands:

• @guidelines-bot /claim - Claim this review for yourself
• @guidelines-bot /label +label-name - Add a label
• @guidelines-bot /label -label-name - Remove a label
• @guidelines-bot /queue - Show reviewer queue
• @guidelines-bot /commands - Show all available commands

Hey @mdabir1203 would you like to claim this review, or let someone else grab it?

@guidelines-bot /claim

✅ @mdabir1203 has claimed this review.

@mdabir1203 is designated as reviewer by queue rotation, but GitHub could not add them to PR Reviewers automatically (API 422). A triage+ approver may still be required before merge queue.

✅ Rectified PR #368: latest review by @mdabir1203 is CHANGES_REQUESTED; refreshed reviewer activity.

⚠️ Review Reminder

Hey @mdabir1203, it's been more than 14 days since you were assigned to review this.

Please take one of the following actions:

1. Begin your review - Post a comment with your feedback
2. Pass the review - Use @guidelines-bot /pass [reason] to assign the next reviewer
3. Step away temporarily - Use @guidelines-bot /away YYYY-MM-DD [reason] if you need time off

If no action is taken within 14 days, you may be transitioned from Producer to Observer status per our contribution guidelines.

Life happens! If you're dealing with something, just let us know.

🔔 Transition Period Ended

@mdabir1203, the 14-day transition period has passed without activity on this review.

Per our contribution guidelines, this may result in a transition from Producer to Observer status.

The review will now be reassigned to the next person in the queue.

If you believe this is in error or have extenuating circumstances, please reach out to the subcommittee.

Any feedbacks

🔔 Transition Period Ended

@mdabir1203, the 14-day transition period has passed without activity on this review.

Per our contribution guidelines, this may result in a transition from Producer to Observer status.

The review will now be reassigned to the next person in the queue.

If you believe this is in error or have extenuating circumstances, please reach out to the subcommittee.

🔔 Transition Period Ended

@mdabir1203, the 14-day transition period has passed without activity on this review.

Per our contribution guidelines, this may result in a transition from Producer to Observer status.

The review will now be reassigned to the next person in the queue.

If you believe this is in error or have extenuating circumstances, please reach out to the subcommittee.